Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds assertions .to.be.defined #440

Closed
wants to merge 2 commits into from
Closed

Conversation

solsson
Copy link

@solsson solsson commented May 6, 2015

There is of course .to.exist and .to.be.ok but since #371 we can no longer recommend that developers end every expectation with () which means we do get no-op expectations now and then. Test-first is a solution of course, but there's also test maintenance.

I didn't understand the rationale for the revert in #306 but I suppose there is good reasons. I think the use of .defined nearby .undefined is the most common mistake when asserting without ().

…otentially harmful in particular when troubleshooting through tests and adding .to.be.defined because you see a .to.be.undefined somehwere. There is .to.exist but after 2.0.0 chaijs#371 assertions are more prone to false positives.
@keithamus
Copy link
Member

Thanks for the PR @solsson.

Firstly, there's a pretty comprehensive history of the whole property/method assertion syntax here.

I must admit I'm a little dubious to include .to.be.defined, as we already have .to.exist (as you suggest). If this is simply to try to alleviate a need for () at every statement, I'd suggest just using the excellent chaining feature that chai has. I personally have linting rules to enforce (), but still happily use should - I'm just a little more careful about how I use it:

expect(foo).to.exist.and.equal(1);
expect(bar).to.exist.and.be.above(3);
expect(baz).to.exist.and.be.a('function');

Usually I find if I'm asserting something exists, I can easily make slightly more specific assertions about it too.

I'll mark this as more-discussion-needed to give you and others a chance to make a case for adding this, but my vote is no for this addition.

@solsson
Copy link
Author

solsson commented May 29, 2015

We do have the same issue that @feugy points out in #371. But we've continued to use chai expect style and with your advice on chaining we do get the advantages of (). Misspelling is only a problem with .be.undefined where chaining is useless.

@keithamus
Copy link
Member

These are the same assertions:

expect(foo).to.be.undefined
expect(foo).to.not.exist
expect(foo).to.equal(undefined);

Feel free to use the one with (), which jshint won't complain about.

I'm still not sold on .to.be.defined - I'm going to close this because of the reasons I mentioned above, but rest assured that I'm also putting thought into how we can alleviate the jshint problems, as this issue is still coming up a lot. For now, you should use dity-chai if you really want method style assertions.

@keithamus keithamus closed this May 29, 2015
@keithamus
Copy link
Member

Also @solsson, please don't let this put you off filing issues and making future PRs. It might be best to discuss your ideas as issues before turning into PRs though, as I'd hate you to feel as though your time is wasted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants